-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update vignettes to snake_case, and other code quality updates #682
Conversation
Note that the alphabetical order will still be displayed in R help pane, no need for it on the website, by topic can be more helpful
since it is not part of the exported API and only used for testing.
…o functions show in the outline.
…n't show up on the function index , but link it in the openxlsx2-deprecated page
Just look at the .Rd, this commit does nothing basically.
… how to use it. The `fmt_txt()` Rd could still be improved imo
I reverted the changes in |
Oh this should be fine. I probably didn't recreate it after the last modifications. Thanks letting me know |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments and questions. I reviewed the changes (aside from the huge write.R ones) and I'm generally fine. Showstoppers remain:
the write changes are still in this pull requestNo they are not! My mistake.- not sure about the wrapper to testthat change. Let's give Jordan a moment - maybe until tomorrow - to comment. After all we can always move the file back to the R folder
Hi, Thanks for the review. I will go through it one last time, ok, let's wait till tomorrow to merge. You may want to double check the vignette with the most changes just to make sure! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are the last places I wanted to highlight
Good to go from my view, thanks a lot! As said, lets give Jordan a few hours to veto the movement of the |
Thanks again for the PR, merged! |
Thanks for reviewing this fast! I think the website looks great: https://janmarvin.github.io/openxlsx2/dev/ I will try switching from openxlsx to openxlsx2 in my personal scripts with the latest dev and will let you know the points of friction! |
Once a pull request gets this large it is blocking everything else. It would have become a pita to update this or other pull requests. Usually it is a better strategy to keep things a tiny bit smaller. But I do not expect any fallout from it and it was way simpler to review than #683 , this one will have to wait a few days. Maybe I can review it over the weekend |
Sorry, I don't think I ever saw this (usually good about checking my mentions) and just happened to notice this while looking at the wrapper datetime failures. From my understanding and use of the Use of the |
Oh, sorry I think I mentioned you somewhere in this PR and wasn't sure if you still cared and didn't want to bother you with repeated mentions. Sorry, if you missed it. Regarding the function, I'll leave this for you guys to decide. I'm fine with either. I was hoping that we one day have a expect_equal() for workbooks, but I'm not seeing this anytime soon and it's not the highest priority right now, because we're currently pretty stable. |
After converting arguments to snake case in #678, this PR updates some of the documentation, along with other code quality improvements.
Addresses #681, #643, #38, follow-up to #678
You may be best suited to review the reference index in
_pkgdown.yml
. before this PR, functions are listed twice. You made a great job of removing exports or adding@keywords internal
in some places to reduce the number of exported functions to simplify the function index.This PR may be a WIP, but may need your views on some topics.
Still not done with reviewing vignettes, not sure how thorough we want to be with row, cols -> dims.
The examples are not done yet.
I didn't have time to go through all vignettes.
When reviewing, please take a look at the rendered .Rd files to make sure nothing got messed up when I removed tags
R/
intests/testthat/helper.R
See https://r-pkgs.org/testing-design.html#sec-testing-design-tension@keywords internal
for internal un-exported functions.For ref, I didn't encounter breakages or R 4.2.3, on Windows!
Edit 2: about
@keywords internal
vs@noRd
the
@keywords internal
I removed were related to unexported functions that already hada@noRd
tagBasically,
@keywords internal
: For exported functions where you don't want the function to show up in the reference index, but still want accessible for thr help?function
(i.e. still create the Rd file)@noRd
Usually For non-exported functions with roxygen documentation, this ensures that no .Rd file is created. therefore@keywords internal
is redundant.I did this cleanup to better represent which functions are exported, but don't show up in the ref index (i.e are not part of the suggested API, or deprecated) but still exported to avoid code breakage.